Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: infer async modules when incremental enabled #7927

Merged
merged 6 commits into from
Sep 20, 2024
Merged

Conversation

ahabhgk
Copy link
Collaborator

@ahabhgk ahabhgk commented Sep 19, 2024

Summary

fix infer async modules when incremental enabled (both incremental and newIncremental)

This could cause a little bit perf regression when cold start but correctness first and the regression is acceptable (under 5% according to the bench) shouldn't cause perf regression

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@github-actions github-actions bot added the team The issue/pr is created by the member of Rspack. label Sep 19, 2024
@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Sep 19, 2024
Copy link

netlify bot commented Sep 19, 2024

Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit 6d392a9
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/66ed52f96d23770008a46e3b

@ahabhgk
Copy link
Collaborator Author

ahabhgk commented Sep 19, 2024

!bench

@rspack-bot
Copy link

rspack-bot commented Sep 19, 2024

📝 Benchmark detail: Open

Name Base (2024-09-19 0a89e43) Current Change
10000_development-mode + exec 2.22 s ± 58 ms 2.23 s ± 40 ms +0.68 %
10000_development-mode_hmr + exec 683 ms ± 3.5 ms 699 ms ± 21 ms +2.34 %
10000_production-mode + exec 2.85 s ± 30 ms 2.83 s ± 31 ms -0.71 %
arco-pro_development-mode + exec 1.81 s ± 72 ms 1.84 s ± 77 ms +1.32 %
arco-pro_development-mode_hmr + exec 433 ms ± 2.3 ms 435 ms ± 2.5 ms +0.56 %
arco-pro_production-mode + exec 3.27 s ± 41 ms 3.28 s ± 50 ms +0.27 %
arco-pro_production-mode_generate-package-json-webpack-plugin + exec 3.29 s ± 97 ms 3.32 s ± 78 ms +0.96 %
threejs_development-mode_10x + exec 1.66 s ± 19 ms 1.68 s ± 10 ms +0.95 %
threejs_development-mode_10x_hmr + exec 791 ms ± 16 ms 788 ms ± 9.5 ms -0.43 %
threejs_production-mode_10x + exec 5.14 s ± 22 ms 5.18 s ± 36 ms +0.74 %

JSerFeng
JSerFeng previously approved these changes Sep 19, 2024
@JSerFeng JSerFeng dismissed their stale review September 19, 2024 10:57

have questions

@JSerFeng
Copy link
Collaborator

Can you explain the issue before ?

@ahabhgk
Copy link
Collaborator Author

ahabhgk commented Sep 19, 2024

for example, a -> b -> c

in the first build, c has top-level-await, this will infer a and b are async modules (a (async) -> b (async) -> c (async))

but in the second build, we remove the top-level-await inside c, the correct module graph should be a -> b -> c, but since we incrementally rebuild the module graph, only rebuild the modified module c, so the module graph is a (async) -> b (async) -> c, this is an incorrect module graph

in this PR we will fix this first in InferAsyncModulesPlugin, we find the not has_top_level_await modules from affected modules, and infer is_async for them, reset is_async back to false first, and then find the has_top_level_await modules from affected modules and set them and there inferred modules to is_async: true

and when newIncremental disabled, old incremental enabled or nothing enabled at all, the affected modules is just all modules, follow the same steps, will also have a correctly updated module graph

@JSerFeng
Copy link
Collaborator

for example, a -> b -> c

in the first build, c has top-level-await, this will infer a and b are async modules (a (async) -> b (async) -> c (async))

Why are a and b not included in affectedModules, as they have data referred from c

@ahabhgk ahabhgk enabled auto-merge (squash) September 20, 2024 10:28
@ahabhgk ahabhgk merged commit e58ac85 into main Sep 20, 2024
28 checks passed
@ahabhgk ahabhgk deleted the fix-infer-async branch September 20, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: bug fix release: bug related release(mr only) team The issue/pr is created by the member of Rspack.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants